-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow to ignore specific dimensions in "shift to zero indexing" #236
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/236/index.html |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #236 +/- ##
==========================================
+ Coverage 92.47% 92.71% +0.23%
==========================================
Files 95 95
Lines 17473 17828 +355
==========================================
+ Hits 16159 16529 +370
+ Misses 1314 1299 -15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good. I've left two remarks about implementation and I think the docstring could be slightly improved maybe.
shift = True | ||
for var in FindVariables().visit(d): | ||
if var in ignore: | ||
shift = False | ||
if shift: | ||
new_dims += [d - sym.Literal(1)] | ||
else: | ||
new_dims += [d] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to propose a slightly streamlined alternative to avoid the explicit loop:
shift = True | |
for var in FindVariables().visit(d): | |
if var in ignore: | |
shift = False | |
if shift: | |
new_dims += [d - sym.Literal(1)] | |
else: | |
new_dims += [d] | |
if ignore and any(var in ignore for var in FindVariables().visit(d)): | |
new_dims += [d] | |
else: | |
new_dims += [d - sym.Literal(1)] |
@@ -33,9 +33,16 @@ | |||
] | |||
|
|||
|
|||
def shift_to_zero_indexing(routine): | |||
def shift_to_zero_indexing(routine, ignore=()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's generally preferred to assign None
as default value for optional arguments, and then convert it to a tuple in the function body, e.g. ignore = as_tuple(ignore)
.
That also adds the benefit that you can do something like shift_to_zero_indexing(routine, ignore='myvar')
and wouldn't get the unexpected behaviour that this is treated as ignore=('m', 'y', 'v', 'a', 'r')
.
routine : :any:`Subroutine` | ||
The subroutine in which the array dimensions should be shifted | ||
ignore : list of str | ||
Dimensions (or rather variables being dimensions) to be ignored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this would be more descriptive?
Dimensions (or rather variables being dimensions) to be ignored | |
List of variable names for which, if found in the dimension expression of an array subscript, that dimension is not shifted to zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comments, all good now!
5dbd1fb
to
1e03830
Compare
1dc82a6
to
12aaeb0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Many thanks for you patience. GTG
allow to ignore specific dimensions ...